Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Maps] Move layers to np maps #61877

Merged
merged 19 commits into from
Apr 6, 2020
Merged

Conversation

kindsun
Copy link
Contributor

@kindsun kindsun commented Mar 30, 2020

Follow-up to #59585. This PR moves layers to NP. For the most part, it really is just physically moving these files and updating refs to the files

@kindsun kindsun added [Deprecated-Use Team:Presentation]Team:Geo Former Team Label for Geo Team. Now use Team:Presentation v8.0.0 release_note:skip Skip the PR/issue when compiling release notes Feature:NP Migration v7.8.0 labels Mar 30, 2020
@kindsun kindsun requested review from a team as code owners March 30, 2020 18:32
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-gis (Team:Geo)

Copy link
Contributor

@thomasneirynck thomasneirynck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for moving! 💦

In general, I think we should strive for as clean a move as possible. It's easier to follow along when the dependencies are legacy->np instead of the other way around. NP is more&more becoming the source-of-truth (especially for utils/configs), so it's iffy referencing from np to legacy when it's not strictly necessary.

  1. There's some unnecessary dependencies from NP to legacy. Instead of referencing legacy, these should reference the original NP-equivalent.
  • `constants
  • kibana_services
  • i18n_getters
  1. While it may be "technically" allowed, I would already start moving legacy module dependencies wholesale, especially the fairly dummy config and utility ones. I don't think there's a particular reason to keep them hanging around in legacy, when we can just move them wholesale right now.
  • legacy/maps/common/descriptor_types: (it pretty much defines the layers/* data-model).
  • elasticsearch_geo_utils
  • connected_components/map/mb_utils (this one actually has a todo anyway).
  1. I'd move over some of the stray utility-functions to NP already (the other utilities can stay in legacy, but just lift out the simple ones).
  • isRetina from meta.

I think I caught most of them. There's a few react-components, and utilities relying on chrome which we can keep in legacy in the short-term.

@kindsun
Copy link
Contributor Author

kindsun commented Mar 30, 2020

Thanks @thomasneirynck. There are several paths that got updated to pull from legacy. I expected this, but usually these get flagged in subsequent linting. It's not at all clear to me why that's not happening here. To resolve these issues and to address the remainder of your comments, I'll need to import a few layers-adjacent files. I'll re-request review when I've completed this work!

Copy link
Contributor

@snide snide left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SCSS files moved only. No need for design review. Did not test functionality of PR.

@thomasneirynck thomasneirynck self-requested a review April 1, 2020 16:14
@kindsun kindsun added the WIP Work in progress label Apr 1, 2020
@kindsun kindsun removed the WIP Work in progress label Apr 3, 2020
@kindsun
Copy link
Contributor Author

kindsun commented Apr 3, 2020

@thomasneirynck @nreese This should be ready for another pass. For the most part, this PR still amounts largely to just moving the layers files to NP, but with a couple of updates:

  • I increased the number of services initialized by NP kibana_services and decreased the number handled by legacy kibana_services since many of our service needs are on the NP side now. This will continue until there are really no kibana_services on the legacy side
  • I added in several files that support layers and aren't really used by legacy. I tried to keep this minimal and really just focused on files layers needs.

Copy link
Contributor

@nreese nreese left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great to see this much code get migrated to NP!

lgtm
code review

Copy link
Contributor

@thomasneirynck thomasneirynck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm when green. np getting there...

@@ -3,7 +3,89 @@
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/
import { esFilters, search } from '../../../../src/plugins/data/public';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great to see so many functions moved over

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@kindsun kindsun merged commit f80925a into elastic:master Apr 6, 2020
@kindsun kindsun deleted the np-move-layers-to-np-maps branch April 6, 2020 20:01
kindsun pushed a commit to kindsun/kibana that referenced this pull request Apr 6, 2020
* Move layers to new location

* Update layer path refs

* Update np kibana services to cover all required services

* Init np kibana services in legacy plugin. Port init functions to np

* Path updates, supporting file moves, general clean up

* More moves of related files and clean-up of legacy refs

* Path updates. Typescript warning fixes

* Update test paths

* Clean up unused kibana services usage in legacy

* Remove unused http ref

* Test fixes and clean up

* Remove unused snapshots

* Add np service init to embeddables too

* Move validate color picker to NP
kindsun pushed a commit that referenced this pull request Apr 6, 2020
* Move layers to new location

* Update layer path refs

* Update np kibana services to cover all required services

* Init np kibana services in legacy plugin. Port init functions to np

* Path updates, supporting file moves, general clean up

* More moves of related files and clean-up of legacy refs

* Path updates. Typescript warning fixes

* Update test paths

* Clean up unused kibana services usage in legacy

* Remove unused http ref

* Test fixes and clean up

* Remove unused snapshots

* Add np service init to embeddables too

* Move validate color picker to NP
gmmorris added a commit to gmmorris/kibana that referenced this pull request Apr 7, 2020
* master: (36 commits)
  [data.search.aggs] Remove service getters from agg types (elastic#61628)
  fixing APM internationalization (elastic#62757)
  fix: 🐛 correctly create error on no_matching_indices (elastic#61257)
  [Lens] Remove all legacy imports (elastic#62596)
  Add label for ace editor (elastic#62588)
  [ML] Show better file structure finder explanations (elastic#62316)
  Fix old pathes in eslintrc (elastic#62580)
  [Uptime] Improve Telemetry test (elastic#62428)
  [SIEM] Adds sort rules Cypress test (elastic#62700)
  [Uptime]Abstracted 'access:uptime-read' tag into a wrapper for… (elastic#62576)
  fixing bug (elastic#62577)
  [Maps] Allow updating requestType for ESGeoGridSource (elastic#62365)
  [Maps] do not show circle border when symbol size is zero (elastic#62644)
  [Maps] Always show current zoom level (elastic#62684)
  bc5 siem rules merge (elastic#62679)
  Revert "[Monitoring] Cluster state watch to Kibana alerting (elastic#61685)"
  Fix visual tests (elastic#62660)
  [Telemetry] update crypto packages (elastic#62469)
  [DOCS] Removed references to left (elastic#60807)
  [Maps] Move layers to np maps (elastic#61877)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Deprecated-Use Team:Presentation]Team:Geo Former Team Label for Geo Team. Now use Team:Presentation Feature:NP Migration release_note:skip Skip the PR/issue when compiling release notes v7.8.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants